Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closes #1604, added simple OVERLAPS support #1611

Merged
merged 2 commits into from
Aug 16, 2022

Conversation

d2a-raudenaerde
Copy link
Contributor

  • Using the ExpressionList approach as discussed in the other PR
  • No renames of existing classes
  • Moved to SQLCondition

@manticore-projects
Copy link
Contributor

Rob,

there was an unrelated PR merged last night, which breaks the tests.
I have provided a fix already, but we will need to wait for it to get merged first.

Until this happens, all our PRs will fail to built. So please a bit patience right now. Thanks, its not in my hands.

@d2a-raudenaerde
Copy link
Contributor Author

I noticed. I spent some time trying to figure out why the AlterTest was failing, until I decided to check if the main branch was the issue, and it was.

(You're still welcome to provide code-review-feedback already of course)

@manticore-projects
Copy link
Contributor

I definitely will, just wait please until Master builds again.
It wasn't me but it affects my work as well.

@manticore-projects
Copy link
Contributor

It's a thing of beauty now and well executed!

One recommendation: You may want to add <OVERLAPS> to the RelObjectWithoutValue Production in order to allow it as Column, Table, Alias, Function. (I still hope that my Keywords PR will be allowed to do that automatically eventually.)

One question: Are those ExpressionList() really of a variable length? Or are they limited to strictly 2? Should we want to enforce it?

Well done and thank you for your contribution!

@d2a-raudenaerde
Copy link
Contributor Author

d2a-raudenaerde commented Aug 4, 2022

Thanks :)

You meant: RelObjectNameWithoutValue right? I'm not really sure if that is correct/something desired? At least PostgreSQL does not allow it to be used as Table without quoting it.

So CREATE TABLE OVERLAPS(A TEXT); gives a syntax error; CREATE TABLE "OVERLAPS"(A TEXT); is allowed.

If there is consensus that it should be allowed, I'm happy to add it! (or maybe, after the keyword PR / discussion is settled?)

About the ExpressionList: I tried to stick to the sql2003 standard. I'm not too confident about the different DBMS handling of OVERLAPS. The standard allows a list of size 2+, or, when prefixed with ROW, a list of size 1+. The latter I did not implement as I tried to follow the mantra 'we will implement it when needed' :)

WDYT?

@manticore-projects
Copy link
Contributor

You meant: RelObjectNameWithoutValue right?
Yes.

I'm not really sure if that is correct/something desired? At least PostgreSQL does not allow it to be used as Table without quoting it.

So CREATE TABLE OVERLAPS(A TEXT); gives a syntax error; CREATE TABLE "OVERLAPS"(A TEXT); is allowed.

As long as it is not explicitly defined by the SQL Standard as a Reserved Keyword we should try to allow it as a keyword.

If there is consensus that it should be allowed, I'm happy to add it! (or maybe, after the keyword PR / discussion is settled?)

I have no particular interest in this one Keyword. But I wanted to point out the mechanism -- for your next great contribution.

About the ExpressionList: I tried to stick to the sql2003 standard. I'm not too confident about the different DBMS handling of OVERLAPS. The standard allows a list of size 2+,

Perfect then, this was all I wanted to know. (I have never used OVERLAPS and I am still very stuck with Oracle and SQL 2002, sorry.)

@d2a-raudenaerde
Copy link
Contributor Author

I have no particular interest in this one Keyword. But I wanted to point out the mechanism -- for your next great contribution.

I will leave this for now, if there arises an issue/request for it, we can easily add it.

Perfect then, this was all I wanted to know

Thanks for the question; I'll leave a comment in the grammar pointing this out, that's useful for other ppl. that might wonder why this has been done this way.

@d2a-raudenaerde
Copy link
Contributor Author

@manticore-projects can this be pulled now? :)

@manticore-projects
Copy link
Contributor

manticore-projects commented Aug 16, 2022 via email

@wumpz wumpz merged commit 236a50b into JSQLParser:master Aug 16, 2022
@wumpz
Copy link
Member

wumpz commented Aug 16, 2022

Did a full beach merge ... :)

@d2a-raudenaerde
Copy link
Contributor Author

Ha, enjoy your beach-time!

@manticore-projects
Copy link
Contributor

You are my hero, Sir!

Aug 18, 2022 8:10:28 PM net.sf.jsqlparser.statement.select.SpecialOracleTest recordSuccessOnSourceFile
INFO: NEW SUCCESS: interval05.sql
Aug 18, 2022 8:10:29 PM net.sf.jsqlparser.statement.select.SpecialOracleTest testAllSqlsParseDeparse
INFO: tested 276 files. got 192 correct parse results, expected 191

You increased the Oracle Coverage/Compliance by 1 test -- and nobody has noticed.
Please go to src/test/java/net/sf/jsqlparser/statement/select/SpecialOracleTest.java and register your succeeding test as I do not want to steal the merit.

Thank you again for the contribution and cheers!

@d2a-raudenaerde
Copy link
Contributor Author

I really don't mind anybody 'stealing' the merit, so go ahead :)

(I just care about having a great tool to help me in my SQL conquests, and am happy to be able to contribute where/when possible)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants